HYPERFLEET-1376 - fix: avoid leaking internal type names in validation errors#306
Conversation
📝 WalkthroughWalkthroughJSON unmarshalling type mismatches now produce structured validation errors naming the affected field and expected JSON kind across shared, force-delete, and create-with-no-content handlers. Other decoding failures retain existing fallback behavior. Patch validation now accepts Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/handlers/framework.go`:
- Around line 159-172: Update describeJSONKind to explicitly map all numeric
reflect.Kind values to "a number", and handle unsupported kinds such as
reflect.Ptr, reflect.Interface, and reflect.Invalid with a safe non-numeric
fallback. Remove the unjustified catch-all behavior so unhandled enum values
cannot produce factually incorrect API errors.
In `@pkg/handlers/validation.go`:
- Around line 253-261: Guard the reflection setup before dereferencing in the
validation flow: validate that reflect.ValueOf(i) is valid, is a pointer or
interface, and is non-nil before calling Elem, returning the existing validation
error path for invalid inputs. Before each IsNil call for Spec, Labels, and
References, only call it for nil-capable kinds (pointer, slice, map, channel,
func, or interface); treat other valid concrete fields as present without
panicking.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: e5824cff-bef1-4940-87e6-d39a49bdbe4d
📒 Files selected for processing (2)
pkg/handlers/framework.gopkg/handlers/validation.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
Risk Score: 0 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 200 lines | +0 |
| Sensitive paths | none | +0 |
| Test coverage | Tests cover changed packages | +0 |
Computed by hyperfleet-risk-scorer
3a2f3c5 to
21a802a
Compare
mliptak0
changed the title
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/handlers/resource_handler_test.go`:
- Around line 279-297: Strengthen the “Success - references only” test’s Patch
expectation by capturing or structurally matching the *api.ResourcePatch
argument instead of accepting any value. Assert that References preserves the
parents entry with kind “Cluster” and ID “cluster-1”, while retaining the
existing successful response and status assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 003292cb-bbd6-4542-8473-1e2155e8f961
📒 Files selected for processing (4)
pkg/handlers/framework.gopkg/handlers/resource_handler_test.gopkg/handlers/resource_status_handler_test.gopkg/handlers/validation_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
| { | ||
| // References-only patch must be accepted, not rejected as "no field provided" — HYPERFLEET-1376 finding 8. | ||
| name: "Success - references only", | ||
| id: "ch-123", | ||
| body: `{"references":{"parents":[{"kind":"Cluster","id":"cluster-1"}]}}`, | ||
| setupMock: func(mock *services.MockResourceService) { | ||
| mock.EXPECT().Patch(gomock.Any(), "Channel", "ch-123", gomock.AssignableToTypeOf(&api.ResourcePatch{})). | ||
| Return(&api.Resource{ | ||
| Meta: api.Meta{ID: "ch-123", CreatedTime: now, UpdatedTime: now}, | ||
| Kind: "Channel", | ||
| Name: "stable", | ||
| Spec: datatypes.JSON(`{"is_default":false}`), | ||
| Generation: 2, | ||
| CreatedBy: "user@test.com", | ||
| UpdatedBy: "user@test.com", | ||
| }, nil) | ||
| }, | ||
| expectedStatusCode: http.StatusOK, | ||
| }, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Assert the references payload passed to Patch.
Line 285 accepts any *api.ResourcePatch, so this passes if decoding or handler wiring drops References before the service call. Capture or match the argument and verify the parents reference, kind, and ID; Sentinel relies on this map/array contract being preserved.
As per path instructions, “New exported functions and critical logic paths SHOULD have tests.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/handlers/resource_handler_test.go` around lines 279 - 297, Strengthen the
“Success - references only” test’s Patch expectation by capturing or
structurally matching the *api.ResourcePatch argument instead of accepting any
value. Assert that References preserves the parents entry with kind “Cluster”
and ID “cluster-1”, while retaining the existing successful response and status
assertions.
Sources: Path instructions, Linked repositories
…errors Sanitizes JSON decode errors across all body-reading handler paths (handle, handleForceDelete, handleCreateWithNoContent) so type mismatches return "field 'x' must be a string" instead of leaking Go struct/package names (e.g. ResourcePatchRequest, openapi.ConditionRequest). Also fixes validatePatchRequest to accept a references-only PATCH, which was previously rejected as "no field provided". Co-Authored-By: Claude <noreply@anthropic.com> Co-authored-by: Cursor <cursoragent@cursor.com>
364f1cb to
a2c06cc
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kuudori The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
28c7a71
into
openshift-hyperfleet:main
2 participants
Summary
What
ResourcePatchRequest,openapi.ConditionRequest) in 400 error messages when a request sends the wrong JSON type forlabelsorconditions.references(nolabels/spec) — these were wrongly rejected with "at least one field must be provided for update".Why
API callers shouldn't see our internal Go type names in error messages, and a references-only PATCH is a perfectly valid update that shouldn't be blocked.
Test Plan
make test-allpassesmake lintpassesmake test-helm(if applicable)